Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use range positions to determine insert_newline motion #9448

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

wmstack
Copy link
Contributor

@wmstack wmstack commented Jan 27, 2024

Fixes #9443

Uses the relative postion of the anchor and the head of a selection to determine whether to shift the selection or to extend it.

@@ -3650,8 +3650,7 @@ pub mod insert {

(pos, pos, local_offs)
};

let new_range = if doc.restore_cursor {
let new_range = if range.head > range.anchor {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also extend if you enter insert mode via o (open_below), O (open_above), or mouse click when in insert mode and then hit enter. Instead I believe we want to check range.cursor(text) > range.anchor to avoid cases where the range is a point (Range::point)

Copy link
Contributor Author

@wmstack wmstack Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. However, I am not too familiar with range.cursor(text). I have run it and it seems to be working fine for my use-cases.

@kirawi kirawi added C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Jan 28, 2024
@the-mikedavis the-mikedavis changed the title use anchor and head positions to determine motion Use range positions to determine insert_newline motion Jan 28, 2024
the-mikedavis
the-mikedavis previously approved these changes Jan 28, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that this is the correct condition. I will test this out locally for a while to see if I can find any cases where we extend unexpectedly

@pascalkuthe
Copy link
Member

pascalkuthe commented Jan 28, 2024

I agree this is correct and inline with how other commands work. I would actually go further: There is only one other place where we use restore_cursor (actually restoring the cursor) I think there we should check range.head > range.anchor && doc.restore_cursor for each selection and only reduce those selections by one where that holds.

@the-mikedavis
Copy link
Member

👍 That would fix the other behavior mentioned in #9443

@wmstack wmstack force-pushed the fix-newline-selection branch 6 times, most recently from 0f15c71 to 19d2e74 Compare January 29, 2024 17:20
the-mikedavis
the-mikedavis previously approved these changes Jan 29, 2024
@the-mikedavis
Copy link
Member

Looks like this needs a run of cargo fmt to pass the lint CI

@wmstack wmstack force-pushed the fix-newline-selection branch from 19d2e74 to 40f0e0e Compare January 29, 2024 18:59
@archseer archseer merged commit cf44921 into helix-editor:master Jan 29, 2024
6 checks passed
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
…9448)

* use anchor and head positions to determine motion

* use range cursor to decide extending or shifting

* add condition to cursor moving back on normal
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
…9448)

* use anchor and head positions to determine motion

* use range cursor to decide extending or shifting

* add condition to cursor moving back on normal
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
…9448)

* use anchor and head positions to determine motion

* use range cursor to decide extending or shifting

* add condition to cursor moving back on normal
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
…9448)

* use anchor and head positions to determine motion

* use range cursor to decide extending or shifting

* add condition to cursor moving back on normal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Odd behaviour with append_mode collapse_selection
5 participants